-
Notifications
You must be signed in to change notification settings - Fork 16
Svls 6036 respect timeouts #537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
alexgallotta
commented
Feb 3, 2025
- use configured flush timeout for traces and stats
- respect the flush timeout, removing tracer retries
- add logs of timing for shipping data
bottlecap/Cargo.toml
Outdated
| datadog-trace-normalization = { git = "https://github.com/DataDog/libdatadog", rev = "eb82b62002a897a8bb9fe1e1cfa49170a9ea6b21" } | ||
| datadog-trace-obfuscation = { git = "https://github.com/DataDog/libdatadog", rev = "eb82b62002a897a8bb9fe1e1cfa49170a9ea6b21" } | ||
| dogstatsd = { git = "https://github.com/DataDog/libdatadog", rev = "eb82b62002a897a8bb9fe1e1cfa49170a9ea6b21" } | ||
| dogstatsd = { git = "https://github.com/DataDog/libdatadog", rev = "5bba5f1415fa07a5a2fc04ec6a3a533909dda1e5" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine but let's bump it to the commit once you merge to main in libdatadog, because my FD leak fix will overwrite this as yours isn't in main yet
b3e15e4 to
19bf675
Compare
19bf675 to
2d6cb7e
Compare
2d6cb7e to
82f99ec
Compare
| url: hyper::Uri::from_str(&stats_url).expect("can't make URI from stats url, exiting"), | ||
| api_key: Some(api_key.clone().into()), | ||
| timeout_ms: Endpoint::DEFAULT_TIMEOUT, | ||
| timeout_ms: config.flush_timeout * 1_000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if flush_timeout is not set? Do we have a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should, it's not an Option
also
https://github.com/DataDog/datadog-lambda-extension/blob/main/bottlecap/src/config/mod.rs#L132
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the DD_FLUSH_TIMEOUT or flush_timeout option in the datadog agent, where does this come from?
| 1, | ||
| 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they come from defaults here
https://github.com/DataDog/libdatadog/blob/main/trace-utils/src/send_with_retry/retry_strategy.rs#L39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you just create it once and clone it another place?
duncanista
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM – left some nits for some magic numbers
astuyve
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go, just drop the debug log